v4.0.0-alpha.1 #113
v4.0.0-alpha.1 #113
Conversation
| func New(rootConfig *rootcmd.RootConfig) *CreateConfig { | ||
| var cfg CreateConfig | ||
| cfg.RootConfig = rootConfig | ||
| cfg.Flags = ff.NewFlags("create").SetParent(cfg.RootConfig.Flags) |
There was a problem hiding this comment.
I don't want to make SetParent part of the Flags interface, because I don't want to dictate that flag sets need to support the concept of parents. At the moment it's a method on the CoreFlags type, and takes a parent CoreFlags value as a parameter. That means that you need to keep track of the concrete CoreFlags value of parent commands somehow, you can't just rely on the Flags field in the parent command directly. At least, not without type assertions. This is annoying, but I'm not sure what a better approach would be.
| // Parse the flag set with the provided args. [Option] values can be used to | ||
| // influence parse behavior. For example, options exist to read flags from | ||
| // environment variables, config files, etc. | ||
| // | ||
| // The fs parameter must be of type [Flags] or [*flag.FlagSet]. Any other type | ||
| // will result in an error. | ||
| func Parse(fs any, args []string, options ...Option) error { |
There was a problem hiding this comment.
I'm not super happy with fs any but I judge that maintaining support for ff.Parse(<*flag.FlagSet>, ...) is important enough to justify complecting the API in this way. I could be convinced otherwise, especially if there were a better design that I haven't thought of...
There was a problem hiding this comment.
I would have loved to use generics to say fs interface{ ff.FlagSet | *flag.FlagSet } — but you can't define a union over anything but concrete types. I guess it makes sense why, but it's unfortunate in this case.
There was a problem hiding this comment.
I agree that seeing any in a signature where there is actually a runtime constraint is uncomfortable, but (for me at least) that's partially due to the permissive connotation. While we can't (yet) be more specific with general interfaces, you might at least be able to sharpen up the perceptual edges with a type alias like:
// AnyFlagSet must be one of { [ff.FlagSet] | [*flag.FlagSet] }
type AnyFlagSet = any
This at least calls out the restriction in a way that jumps out in the godoc, which could be good enough for now:
go doc Parse
package ff // import "."
func Parse(fs AnyFlagSet, args []string, options ...Option) error
Parse the flag set with the provided args. Option values can be used to
influence parse behavior. For example, options exist to read flags from
environment variables, config files, etc.
The fs parameter must be of type Flags or *flag.FlagSet. Any other type will
result in an error.
go doc AnyFlagSet
package ff // import "."
type AnyFlagSet = any
AnyFlagSet must be one of { ff.FlagSet | *flag.FlagSet }
There was a problem hiding this comment.
Or, alternatively, FlagSetAny, depending on whether you think of it as "an any that's a flagset" or "[the] FlagSet types' any" :D
| // Flags is the set of flags associated with, and parsed by, this command. | ||
| // | ||
| // When building a command tree, it's often useful to allow flags defined by | ||
| // parent commands to be specified by any subcommand. The core flag set | ||
| // supports this behavior via SetParent, see the documentation of that | ||
| // method for details. | ||
| // | ||
| // Optional. If not provided, an empty flag set will be constructed and used | ||
| // so that the -h, --help flag works as expected. | ||
| Flags Flags |
There was a problem hiding this comment.
Command.Flags needs to be a true Flags, meaning you can't pass a flag.FlagSet directly, you have to use the adapter constructor. I think this is reasonable: ff.Parse is meant to be symmetrical with flag.Parse, but ff.Command doesn't have any direct analogue in that sense.
| // GetParent returns the parent command of this command, or nil if a parent | ||
| // hasn't been set. Parents are set during the parse phase, but only for | ||
| // commands which are traversed. | ||
| func (cmd *Command) GetParent() *Command { | ||
| return cmd.parent | ||
| } |
There was a problem hiding this comment.
The intent here is to support usage functions that want to include info from parent commands in the tree.
|
More of a drive-by. Are you looking for feedback on this I've used the |
|
Feedback is very much appreciated! |
peterbourgon
force-pushed
the
v4-alpha.1
branch
from
8cbce4e
to
fe2014a
Compare
peterbourgon
force-pushed
the
v4-alpha.1
branch
from
fe2014a
to
9d2e56d
Compare
|
I'm going to go ahead and release this as |
This PR introduces v4 of package ff, which includes several enhancements and breaking changes.
What are the major changes?
Why move away from flag.FlagSet?
Basically, it was too limited.
While I'm not necessarily sold on all of getopts(3), I think allowing flags to have long (--foo) and short (-f) names is an important feature, and a basic expectation for nearly all users. The fact that the stdlib flag set doesn't support this feature proved to be a problem worth fixing.
Also, using a concrete type kind of paints the module into a corner, in terms of capability and extensibility. I don't think it would have been possible to provide the most-requested features (hierarchical flags, support for different flag set implementations, etc.) without switching to some kind of abstraction.
Why adapt a flag.FlagSet to a bespoke CoreFlags instead of using a separate type?
To take advantage of CoreFlags features like Reset and SetParent, without needing to re-implement them. (I'm not totally convinced this is the right approach, but it's where I've landed for the moment.)
Why model hierarchy as parent flag sets instead of multiple flag sets in a command?
You always have a single set of args provided by the user. If you want to apply those args to multiple flag sets, then you need to find a way for the Parse method of each flag set to consume only the args that map to flags known to the flag set, and yield the args that map to flags that aren't known to the flag set, while also returning non-flag args separately. The command parser would then need to call Parse for each flag set in sequence, passing down the leftover args after each stage.
This would require changes to the Parse method in the Flags interface, which is currently extremely good and intuitive:
Parse(args []string) error. Complicating it would be a very tall order, and a large burden for implementations. And after a lot of experimentation, I concluded that it was infeasible to reliably distinguish "a flag that we don't know about" from "a non-flag argument" when parsing args.Consequently, the design keeps the simpler Parse signature in the Flags interface, and moves all of the logic related to parent flags into the implementation(s). The GetFlags method in the Flag interface was added in part so that callers can know in which flag set a flag was defined.